fix(auto-parser): support bare bracket tool calls#305
fix(auto-parser): support bare bracket tool calls#305Thump604 merged 1 commit intowaybarrios:mainfrom
Conversation
janhilgard
left a comment
There was a problem hiding this comment.
Review: fix(auto-parser): support bare bracket tool calls
Overall: Correct feature addition with good test coverage, but the streaming detection logic has a concerning complexity issue.
What this does
Adds support for the [func_name({...})] bare bracket tool call format to the AutoToolParser. This format is used by some models that do not prefix with [Calling tool: or use XML/Mistral-style markup.
Strengths
-
Clean pattern addition in
extract_tool_calls. TheBARE_BRACKET_PATTERNis inserted at the right position in the priority chain (after Qwen bracket, before Nemotron). The pattern\[(\w+)\((\{.*?\})\)\]correctly captures the function name and JSON arguments. -
Good test coverage. Three new tests:
test_detects_bare_bracketfor non-streaming extractiontest_auto_streaming_bare_bracketfor incremental streamingtest_auto_parser_streams_bare_bracket_tool_callsfor end-to-end server streaming
-
Server-side streaming detection.
_streaming_tool_markup_possibleis added to detect bare bracket patterns in accumulated text, replacing the previous"<" in contentcheck.
Issues
-
The
has_markerlogic inextract_tool_calls_streamingis convoluted and likely buggy.has_marker = any(m in current_text for m in markers) and ( self.BARE_BRACKET_PARTIAL_PATTERN.search(current_text) is not None or self.BARE_BRACKET_PATTERN.search(current_text) is not None or "[Calling tool:" in current_text or self.MISTRAL_TOKEN in current_text or "<" in current_text )
The
markerslist now includes"["which matches nearly any text containing a square bracket (e.g. JSON arrays in content, markdown links). The second condition then acts as a guard, but the overall logic is hard to reason about.Specific bug: A model output like
"See [this link](url)"would match"["in markers, then the second conjunction falls through to"<" in current_textwhich would be false (no angle brackets), but if any of the other conditions happen to match, it would incorrectly trigger tool parsing. The intent seems right but the expression is fragile. -
BARE_BRACKET_PARTIAL_PATTERNearly return can suppress legitimate tool detections.if ( self.BARE_BRACKET_PARTIAL_PATTERN.search(current_text) is not None and self.BARE_BRACKET_PATTERN.search(current_text) is None ): return None
This returns
None(meaning "not content, not tool call -- buffer") when a partial bare bracket is detected but the full pattern is not complete. This is correct for actual bare bracket tool calls, but could incorrectly suppress streaming output if the model produces text like[read(followed by something that is not a JSON object. -
Ordering concern with Qwen bracket pattern.
QWEN_BRACKET_PATTERNis\[Calling tool:\s*(\w+)\((\{.*?\})\)\]andBARE_BRACKET_PATTERNis\[(\w+)\((\{.*?\})\)\]. Since Qwen bracket is checked first (step 3) and bare bracket second (step 4), this is fine. But the bare bracket pattern would also match Qwen bracket format text, so if the ordering ever changes, there would be double-counting. -
The
_streaming_tool_markup_possiblein server.py overlaps with PR #304. Both PRs modify the same streaming fast-path check inserver.py. These will conflict on merge. Consider coordinating the merge order.
Minor
- The renumbering of steps (4 -> 5, 5 -> 6, etc.) in
extract_tool_callsis correct and helpful for readability. - The
_STREAMING_BARE_BRACKET_PARTIALregex could be more precise --\[\w+\($only anchors at end of string, which is correct for streaming but could miss patterns mid-text in non-streaming contexts.
The core feature is useful and the non-streaming path is clean. The streaming detection logic needs simplification -- the compound boolean with "[" as a marker is the main concern.
89f1856 to
5bd9c2d
Compare
5bd9c2d to
86d11f2
Compare
Teach the auto tool parser to recognize bare bracket tool calls like
[read({"file_path": "/tmp/test.py"})]. Add _streaming_tool_markup_possible()
to server.py so the streaming fast-path wakes on bare bracket markup
instead of only on '<'.
Closes waybarrios#146.
86d11f2 to
cd79ecf
Compare
Summary
[read({"file_path": "/tmp/test.py"})]Test plan
PYTHONPATH=/tmp/vllm-mlx-issue146 /opt/ai-runtime/venv-live/bin/python -m pytest /tmp/vllm-mlx-issue146/tests/test_tool_parsers.py -qPYTHONPATH=/tmp/vllm-mlx-issue146 /opt/ai-runtime/venv-live/bin/python -m pytest /tmp/vllm-mlx-issue146/tests/test_server.py -q/opt/ai-runtime/venv-live/bin/python -m black --check --fast /tmp/vllm-mlx-issue146/vllm_mlx/tool_parsers/auto_tool_parser.py /tmp/vllm-mlx-issue146/vllm_mlx/server.py /tmp/vllm-mlx-issue146/tests/test_tool_parsers.py /tmp/vllm-mlx-issue146/tests/test_server.pyCloses #146.